Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace ValueCow::as_bool with truthiness function #17

Merged
merged 4 commits into from
Jul 1, 2023
Merged

Replace ValueCow::as_bool with truthiness function #17

merged 4 commits into from
Jul 1, 2023

Conversation

lunabunn
Copy link
Contributor

Not sure if you actually wanted this, but I felt that it could be useful for my application, so:
Implements a truthiness function for evaluating conditionals.

The following values are falsy:

  • None
  • false
  • 0 integer and 0.0 float
  • empty string, list, and map

Everything else is truthy.

Closes #9. Please let me know if there is anything you would like to see changed!

@lunabunn
Replace ValueCow::as_bool with truthiness function
2cc52bd 
Implements a truthiness function for evaluating conditionals.

The following values are falsy:
- None
- `false`
- `0` integer and `0.0` float
- empty string, list, and map

Everything else is truthy.
rossmacarthur
rossmacarthur reviewed May 21, 2023
View reviewed changes
Copy link
Owner

@rossmacarthur rossmacarthur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what I didn't put in the original issue #9 is that imagined the truthiness to be configurable. I think my original thoughts were that it makes template logic more robust to enforce booleans in if statements. Although I don't have any real data to back this up, if you think this is what most people will want then I'm happy to make this the default.

src/render/value.rs Show resolved Hide resolved
@lunabunn
Copy link
Contributor Author

lunabunn commented May 23, 2023 •
edited
Loading

I think what I didn't put in the original issue #9 is that imagined the truthiness to be configurable. I think my original thoughts were that it makes template logic more robust to enforce booleans in if statements. Although I don't have any real data to back this up, if you think this is what most people will want then I'm happy to make this the default.

I would agree that "strict" conditional evaluation can be a good thing, but the merit of that seems a bit lost on upon because it lacks builtin filters for these comparisons (making the convenience-correctness tradeoff not worth it, in my opinion).

Maybe we should take this opportunity to investigate #6?

In the meantime, I think it's a good idea to have this be a configurable setting, whichever the default may be. Off the top of my head, there could be a BooleanEval enum with Strict, Truthy, and Custom(Box<dyn Fn(expr, span) -> Result<bool, Error>>) that the user can set on the Engine? Or should this be another template-specific setting (#15)?

@rossmacarthur rossmacarthur mentioned this pull request May 23, 2023
Open
@rossmacarthur
Copy link
Owner

In the meantime, I think it's a good idea to have this be a configurable setting

I think we can discuss the in a separate issue #19 . I think let's get the build passing on this change and then we can merge it like it is now.

@rossmacarthur
Copy link
Owner

@lunabunn Needed this today, so I fixed the build and I'm going to merge if that's okay

@rossmacarthur rossmacarthur merged commit a4d3c59 into rossmacarthur:trunk Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rossmacarthur rossmacarthur rossmacarthur left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider adding a truthiness function
2 participants
@lunabunn @rossmacarthur